Add capability to write signal with unique `samps_per_frame` to `wfdb.io.wrsamp` #510

briangow · 2024-10-16T22:00:12Z

This PR adds the capability for writing signals with unique samples per frame (samps_per_frame) to wfdb.io.wrsamp. This is typically the function that is used to write WFDB files. This was previously only possible to do by creating a Record first and using its wrsamp method to do the write.

@bemoody , I do feel like checking that the frames were uniform took a bit of extra code. I'm happy to leave it like this but also open to discussing other approaches.

I've added a couple of tests to check that this continues to work as expected.

tompollard · 2024-10-16T23:06:09Z

wfdb/io/record.py

-    if p_signal is not None and d_signal is not None:
+    signal_list = [p_signal, d_signal, e_p_signal, e_d_signal]
+    signals_set = sum(1 for var in signal_list if var is not None)
+    if signals_set != 1:


Is this intentionally changing behavior for calls where p_signal=None and d_signal=None? In the past, this would evaluate to False when e_p_signal=None and e_d_signal=None, but it now evaluates to True.

@tompollard , if all signals are passed as None a failure will occur further down the line. @bemoody , do we want to allow all signals to be None when calling wfdb.io.wrsamp? If we don't want to allow that then I will leave the code as I have it which requires that one of the signals is set.

If passing None for both signals (p_signal and d_signal) previously resulted in an error, then the new behavior is an improvement. Unless people were intentioanlly calling the function with both signals set to None previously, this change isn't a problem.

tompollard · 2024-10-17T02:05:11Z

wfdb/io/record.py

-    # required features.
+
+    # If samps_per_frame is a list, check that it aligns as expected with the channels in the signal
+    if len(samps_per_frame) > 1:


If samps_per_frame is not a list (e.g. it's an int), this will error out.

len(4)
TypeError: object of type 'int' has no len()

tompollard · 2024-10-17T02:21:16Z

wfdb/io/record.py

+            raise TypeError("Unsupported signal format. Must be ndarray or list of lists.")
+
+        # Check that the number of channels matches the number of samps_per_frame entries
+        if num_sig_channels != len(samps_per_frame):


as above, len(samps_per_frame) will error if samps_per_frame is an int.

wfdb/io/record.py

…_frame

briangow · 2024-10-18T18:29:50Z

@tompollard , if this failing test is of concern, could you guide me on what to do to resolve it:

would reformat /home/runner/work/wfdb-python/wfdb-python/wfdb/io/record.py
2 files would be reformatted, 36 files would be left unchanged.

tompollard · 2024-10-18T19:08:19Z

@tompollard , if this failing test is of concern, could you guide me on what to do to resolve it:

It looks like the style checks are failing (https://github.com/MIT-LCP/wfdb-python/actions/runs/11408864264/job/31747939807?pr=510). To fix, you'll need to make sure that the code conforms to black (install black, run black over the code to reformat).

bemoody · 2024-10-21T20:05:33Z

The following raises an error:

>>> wfdb.wrsamp("xxx", fs=500, units=["mV"], sig_name=["I"], fmt=["16"],
                e_p_signal=[numpy.array([0.0, 1.0, 2.0])],
                adc_gain=[1.0], baseline=[0])
TypeError: 'NoneType' object is not subscriptable

In this case, wrsamp should raise an exception to say that if e_p_signal is provided, then samps_per_frame must be too.

bemoody · 2024-10-21T20:05:50Z

The following raises an error:

>>> wfdb.wrsamp("xxx", fs=500, units=["mV"], sig_name=["I"], e_p_signal=[numpy.array([0.0, 1.0, 2.0])], fmt=["16"], samps_per_frame=[3])
TypeError: ufunc 'isinf' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

The problem here is that calc_adc_params doesn't handle the expanded case. calc_adc_params should use the same method to generate adc_gain and baseline as it does in the non-expanded case.

I think it should be straightforward to restructure calc_adc_params to take an expanded argument and to use e_p_signal instead of p_signal where appropriate.

briangow · 2024-10-28T19:20:08Z

The following raises an error:
>>> wfdb.wrsamp("xxx", fs=500, units=["mV"], sig_name=["I"], fmt=["16"],
                e_p_signal=[numpy.array([0.0, 1.0, 2.0])],
                adc_gain=[1.0], baseline=[0])
TypeError: 'NoneType' object is not subscriptable
In this case, wrsamp should raise an exception to say that if e_p_signal is provided, then samps_per_frame must be too.

@bemoody , thanks for catching this. I think this also applies for e_d_signal. I've added a check to make sure samps_per_frame is supplied when writing these expanded signals.

I've also updated the check to make sure samps_per_frame aligns with the channels in the signal, to consider the case when samps_per_frame is an int instead of a list.

Finally, I think we should allow passing samps_per_frame with a 1 for each channel to wrsamp when a mixed frequency signal is read in with smooth_frames=True:

rec = wfdb.io.rdrecord('/wfdb-python/sample-data/mixedsignals', smooth_frames=True)

print(rec.samps_per_frame)
[4, 4, 4, 2, 2, 1]

wfdb.io.wrsamp('written_mixedsignals', fs = rec.fs, units=rec.units, sig_name=rec.sig_name, base_date=rec.base_date, base_time=rec.base_time, comments=rec.comments, p_signal=rec.p_signal, samps_per_frame=[1,1,1,1,1,1], baseline=rec.baseline, adc_gain=rec.adc_gain, fmt=rec.fmt)

Please let me know if you disagree, or have other thoughts with respect to this case.

briangow · 2024-10-29T14:57:02Z

The following raises an error:
>>> wfdb.wrsamp("xxx", fs=500, units=["mV"], sig_name=["I"], e_p_signal=[numpy.array([0.0, 1.0, 2.0])], fmt=["16"], samps_per_frame=[3])
TypeError: ufunc 'isinf' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
The problem here is that calc_adc_params doesn't handle the expanded case. calc_adc_params should use the same method to generate adc_gain and baseline as it does in the non-expanded case.

I think it should be straightforward to restructure calc_adc_params to take an expanded argument and to use e_p_signal instead of p_signal where appropriate.

@bemoody , this is being addressed in a separate PR: #512 .

briangow requested a review from bemoody October 16, 2024 22:00

tompollard reviewed Oct 17, 2024

View reviewed changes

briangow added 4 commits October 18, 2024 14:09

update wfdb.io.wrsamp to allow writing a signal with unique samps_per…

1a9937f

…_frame

add tests for wfdb.io.wrsamp for a signal with unique samps_per_frame

d7b5e93

address failing tests and feedback

0e32209

correct signal name being set

def62e9

briangow force-pushed the bg_wrsamp_multiple_fs branch from 866fe10 to def62e9 Compare October 18, 2024 18:11

add missing import

11e8756

briangow added 2 commits October 19, 2024 10:01

reformatted with black package

f8084d3

reformatted with black package

1e93bee

add wfdb.io.wrsamp signal checks

7bc468b

reformatted with black package

6b0d317

bemoody mentioned this pull request Oct 31, 2024

Allow expanded physical signal in calc_adc_params #512

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add capability to write signal with unique `samps_per_frame` to `wfdb.io.wrsamp` #510

Add capability to write signal with unique `samps_per_frame` to `wfdb.io.wrsamp` #510

briangow commented Oct 16, 2024

tompollard Oct 16, 2024 •

edited

Loading

briangow Oct 18, 2024

tompollard Oct 18, 2024

tompollard Oct 17, 2024

briangow Oct 18, 2024

tompollard Oct 17, 2024

briangow Oct 18, 2024

briangow commented Oct 18, 2024

tompollard commented Oct 18, 2024 •

edited

Loading

bemoody commented Oct 21, 2024

bemoody commented Oct 21, 2024

briangow commented Oct 28, 2024

briangow commented Oct 29, 2024

Add capability to write signal with unique samps_per_frame to wfdb.io.wrsamp #510

Are you sure you want to change the base?

Add capability to write signal with unique samps_per_frame to wfdb.io.wrsamp #510

Conversation

briangow commented Oct 16, 2024

tompollard Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

briangow Oct 18, 2024

Choose a reason for hiding this comment

tompollard Oct 18, 2024

Choose a reason for hiding this comment

tompollard Oct 17, 2024

Choose a reason for hiding this comment

briangow Oct 18, 2024

Choose a reason for hiding this comment

tompollard Oct 17, 2024

Choose a reason for hiding this comment

briangow Oct 18, 2024

Choose a reason for hiding this comment

briangow commented Oct 18, 2024

tompollard commented Oct 18, 2024 • edited Loading

bemoody commented Oct 21, 2024

bemoody commented Oct 21, 2024

briangow commented Oct 28, 2024

briangow commented Oct 29, 2024

Add capability to write signal with unique `samps_per_frame` to `wfdb.io.wrsamp` #510

Add capability to write signal with unique `samps_per_frame` to `wfdb.io.wrsamp` #510

tompollard Oct 16, 2024 •

edited

Loading

tompollard commented Oct 18, 2024 •

edited

Loading